-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(db, webserver): Implement GitHubRepositoryProvider connect flow #1749
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1749 +/- ##
==========================================
- Coverage 54.38% 53.11% -1.28%
==========================================
Files 117 121 +4
Lines 10227 10476 +249
==========================================
+ Hits 5562 5564 +2
- Misses 4665 4912 +247 ☔ View full report in Codecov by Sentry. |
ac4ba59
to
0623d09
Compare
@@ -0,0 +1,2 @@ | |||
DROP TABLE github_repository_provider; | |||
DROP TABLE github_provided_repositories; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove? BTW - can this type of error captured in unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already removed, and I'm not sure - we could have a CI tool which runs cargo sqlx db reset --source ee/tabby-db/migrations
and then tries to de-migrate with cargo sqlx migrate revert
, which should error if the down migration is invalid. I don't think a unit test could catch it, though.
284eb72
to
86d2973
Compare
0407350
to
cc612aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LG!
@@ -226,6 +230,30 @@ impl Query { | |||
.await | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still need api for C/U/D? but it's fine to do it in a follow up PR
|
||
fn get_authorize_url(client_id: &str, redirect_uri: &str, id: &ID) -> Result<Url> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn get_authorize_url(client_id: &str, redirect_uri: &str, id: &ID) -> Result<Url> { | |
// FIXME(boxbeam): refactoring the logic using `/oauth/github.rs` | |
fn get_authorize_url(client_id: &str, redirect_uri: &str, id: &ID) -> Result<Url> { |
|
||
async fn exchange_access_token( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async fn exchange_access_token( | |
// FIXME(boxbeam): refactoring the logic using `/oauth/github.rs` | |
async fn exchange_access_token( |
Relates to TAB-479